Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Bigint backend pt 3: Move ec point conversion to TS #140

Merged
merged 9 commits into from
Sep 22, 2023
Merged

Bigint backend pt 3: Move ec point conversion to TS #140

merged 9 commits into from
Sep 22, 2023

Conversation

mitschabaude
Copy link
Collaborator

@mitschabaude mitschabaude commented Sep 12, 2023 •
edited
Loading

Incremental step on top of #139

This PR part of a multi-PR series to move JSOO / Wasm conversion methods from bindings.js to TS:

  • Conversion of wires, gates, and different kinds of field vector
  • Conversion of curve points and polynomial commitments
  • Conversion of verification key
  • Conversion of oracles
  • Conversion of proof
  • Cleanup conversion modules

@mitschabaude mitschabaude requested a review from a team as a code owner September 12, 2023 10:27
@mitschabaude mitschabaude mentioned this pull request Sep 12, 2023
Merged
6 tasks
Base automatically changed from perf/bigint-backend-2 to main September 13, 2023 08:16
dannywillems
dannywillems reviewed Sep 19, 2023
View reviewed changes
kimchi/js/bindings.js Outdated Show resolved Hide resolved
@dannywillems
Copy link
Member

dannywillems commented Sep 19, 2023 •
edited
Loading

Discussed offline with Gregor: I would be in favor or always having the corresponding o1js PR to have the tests and be sure the code is correct. Atm, we only use a linter in the CI. As the code is moved to pure TS, tests could be added. That would be important IMO.
I reviewed by looking at the correspondances between the JS and TS code.
Gregor told me the final PR tests everything. I can accept the PR as it is, modulo my comments above.

I am also not a TS expert, therefore there might be some optimisations/cleaner code that could be written.

@dannywillems
Copy link
Member

dannywillems commented Sep 19, 2023 •
edited
Loading

Discussed offline with Gregor: I would be in favor or always having the corresponding o1js PR to have the tests and be sure the code is correct. Atm, we only use a linter in the CI. As the code is moved to pure TS, tests could be added. That would be important IMO. I reviewed by looking at the correspondances between the JS and TS code. Gregor told me the final PR tests everything. I can accept the PR as it is, modulo my comments above.

I would open a follow-up issue for the tests in TS

@dannywillems
Copy link
Member

Is it possible to add in the CI the code typechecks?

@mitschabaude
Copy link
Collaborator Author

Discussed offline with Gregor: I would be in favor or always having the corresponding o1js PR to have the tests and be sure the code is correct

Running CI with o1js main + the latest version of this PR here: o1-labs/o1js#1117

@mitschabaude
Copy link
Collaborator Author

Is it possible to add in the CI the code typechecks?

that would be very nice indeed, will think about it. probably just means an extra tsconfig file here and make CI install TS

This was referenced Sep 22, 2023
Open
Open
mitschabaude added a commit to o1-labs/o1js that referenced this pull request Sep 22, 2023
@mitschabaude
bindings for o1-labs/o1js-bindings#140
a818b78
@dannywillems dannywillems mentioned this pull request Sep 22, 2023
Open
@mitschabaude mitschabaude merged commit 5174c22 into main Sep 22, 2023
1 check passed
@mitschabaude mitschabaude deleted the perf/bigint-backend-3 branch September 22, 2023 12:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rbonichon rbonichon rbonichon left review comments

@dannywillems dannywillems dannywillems approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@mitschabaude @dannywillems @rbonichon